Skip to content

Onboarding v4: design feedback round 2#2440

Open
noisysocks wants to merge 11 commits intomainfrom
randerson/onboarding-v4-design-feedback-2
Open

Onboarding v4: design feedback round 2#2440
noisysocks wants to merge 11 commits intomainfrom
randerson/onboarding-v4-design-feedback-2

Conversation

@noisysocks
Copy link
Contributor

@noisysocks noisysocks commented Mar 12, 2026

Asana Task/Github Issue:
https://app.asana.com/1/137249556945/project/1209121419454298/task/1213627872900762?focus=true
https://app.asana.com/1/137249556945/project/1142021229838617/task/1213508111481481?focus=true

Description

Address design feedback on the v4 onboarding flow:

  • Left align StepHeader text — title and subtitle now align left instead of center
  • Fix video flash when toggling DuckPlayer — use requestVideoFrameCallback to wait for the first frame before switching visibility, preventing a flash of the previous frame
  • Stagger bubble fade-in — top bubble (title/subtitle) fades in 100ms before the bottom bubble (body content), sequencing the load-in to guide the user's eye

Testing Steps

https://rawcdn.githack.com/duckduckgo/content-scope-scripts/693139ddb5fdd849f3efe161333ac64f06de9d81/build/integration/pages/onboarding/index.html?order=v4

  • Navigate through the onboarding steps and observe that titles appear slightly before body content on each transition
  • On the Duck Player step, toggle between "with" and "without" and verify no flash of the previous video frame
  • Verify StepHeader text is left-aligned

Checklist

  • I have tested this change locally
  • I have tested this change locally in all supported browsers
  • This change will be visible to users
  • I have added automated tests that cover this change
  • I have ensured the change is gated by config
  • This change was covered by a ship review
  • This change was covered by a tech design
  • Any dependent config has been merged

🤖 Generated with Claude Code


Note

Low Risk
Low risk UI/animation tweaks limited to onboarding v4 styling and media playback timing; main risk is minor timing/visibility regressions across browsers.

Overview
Applies v4 onboarding design feedback by left-aligning key text blocks (e.g. StepHeader, Title, and Get Started body copy) instead of centering.

Adds configurable staggered fade-in timing for bubbles by introducing a fadeInDelay prop on Bubble (CSS var-driven), using it in SingleStep to delay the bottom bubble when a top bubble is present, and adding a staged fade-in wrapper for Make Default content (with a bubbleFadeInDelay query-param override).

Fixes Duck Player toggle flashing by waiting for the first rendered frame via requestVideoFrameCallback before swapping video visibility, switching hidden video styling to opacity: 0/pointer-events: none, and updating the Playwright assertion accordingly.

Written by Cursor Bugbot for commit fee4e6d. This will update automatically on new commits. Configure here.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

Build Branch

Branch pr-releases/randerson/onboarding-v4-design-feedback-2
Commit c5b08ec9f2
Updated March 13, 2026 at 4:13:55 AM UTC

Static preview entry points

QR codes (mobile preview)
Entry point QR code
Docs QR for docs preview
Static pages QR for static pages preview
Integration pages QR for integration pages preview

Integration commands

npm (Android / Extension):

npm i github:duckduckgo/content-scope-scripts#pr-releases/randerson/onboarding-v4-design-feedback-2

Swift Package Manager (Apple):

.package(url: "https://github.com/duckduckgo/content-scope-scripts.git", branch: "pr-releases/randerson/onboarding-v4-design-feedback-2")

git submodule (Windows):

git -C submodules/content-scope-scripts fetch origin pr-releases/randerson/onboarding-v4-design-feedback-2
git -C submodules/content-scope-scripts checkout origin/pr-releases/randerson/onboarding-v4-design-feedback-2
Pin to exact commit

npm (Android / Extension):

npm i github:duckduckgo/content-scope-scripts#c5b08ec9f26b374f41757a6e00db0575efacd7d2

Swift Package Manager (Apple):

.package(url: "https://github.com/duckduckgo/content-scope-scripts.git", revision: "c5b08ec9f26b374f41757a6e00db0575efacd7d2")

git submodule (Windows):

git -C submodules/content-scope-scripts fetch origin pr-releases/randerson/onboarding-v4-design-feedback-2
git -C submodules/content-scope-scripts checkout c5b08ec9f26b374f41757a6e00db0575efacd7d2

@github-actions github-actions bot added the semver-patch Bug fix / internal — no release needed label Mar 12, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

[Beta] Generated file diff

Time updated: Fri, 13 Mar 2026 04:14:24 GMT

Apple
    - apple/pages/onboarding/dist/index.css
  • apple/pages/onboarding/dist/index.js

File has changed

Integration
    - integration/pages/onboarding/dist/index.css
  • integration/pages/onboarding/dist/index.js

File has changed

Windows
    - windows/pages/onboarding/dist/index.css
  • windows/pages/onboarding/dist/index.js

File has changed

@github-actions
Copy link
Contributor

⚠️ Cursor review was not successful.

This PR requires a manual review and approval from a member of one of the following teams:

  • @duckduckgo/content-scope-scripts-owners
  • @duckduckgo/apple-devs
  • @duckduckgo/android-devs
  • @duckduckgo/team-windows-development
  • @duckduckgo/extension-owners
  • @duckduckgo/config-aor
  • @duckduckgo/breakage-aor
  • @duckduckgo/breakage

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Web Compatibility Assessment

  1. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (L95-L101) — severity: error
    video.requestVideoFrameCallback(...) is called unguarded. If this API is unavailable in any supported runtime/WebView build, play() rejects before state transition logic runs, causing toggle/ended handlers to fail (and likely emit unhandled promise rejections). This maps to Platform-Specific Breakage / Missing APIs.

  2. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (L95-L101, L133-L144) — severity: warning
    frameReady is awaited even when video.play() throws. In failure modes where playback never starts (decode/network/autoplay edge cases), the frame callback may never fire, leaving toggle()/end() suspended and UI state stuck. This is a Timing/Race compatibility risk because transition completion now depends on a callback that may not resolve.

  3. special-pages/pages/onboarding/app/v4/components/StepHeader.module.css (L19, L32), special-pages/pages/onboarding/app/v4/components/Bubble.js (L147), special-pages/pages/onboarding/app/v4/components/Bubble.module.css (L40) — severity: info
    These visual-only changes (left alignment and staggered fade delay via CSS custom property) are low compatibility risk and do not alter browser API contracts.

Security Assessment

No direct security findings in this diff.

  • No changes to captured globals, wrapper utilities, message bridge, origin validation, or transport code.
  • No new postMessage, network fetch/XHR, dynamic code execution, or config-trust boundary changes.
  • Changed logic is isolated to onboarding special-page UI behavior.

Risk Level

Medium Risk — UI-only scope, but the new requestVideoFrameCallback dependency introduces a real cross-runtime compatibility failure mode that can block step transitions.

Recommendations

  1. Add capability/error hardening around first-frame wait: guard requestVideoFrameCallback and ensure play() always resolves/rejects deterministically (do not await an unresolvable frame promise).
  2. Add an integration test path that simulates missing requestVideoFrameCallback and a video.play() rejection, and assert the toggle flow still reaches a settled state.
  3. Catch/handle async handler failures in toggle()/end() call paths to prevent unhandled promise rejection noise and silent stuck states.
Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

Base automatically changed from randerson/onboarding-v4-polish to main March 12, 2026 09:18
noisysocks and others added 4 commits March 12, 2026 20:24
Use requestVideoFrameCallback to delay visibility switch until the
incoming video's first frame is painted. Use a state ref so the
onEnded handler always reads fresh state. Hide inactive video with
opacity: 0 instead of visibility: hidden so the compositor keeps
decoding frames.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When transitioning between steps, the top bubble (title/subtitle)
now fades in 100ms before the bottom bubble (body content). This
sequences the load-in to guide the user's eye and improve
comprehension.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@noisysocks noisysocks force-pushed the randerson/onboarding-v4-design-feedback-2 branch from a7a4953 to b59639f Compare March 12, 2026 09:25
@noisysocks noisysocks marked this pull request as ready for review March 12, 2026 09:27
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Web Compatibility Assessment

  1. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (approx. lines 95-101) — severity: error
    video.requestVideoFrameCallback(...) is called unconditionally. This API is not universally available across all embedded WebView/Safari baselines, so play() can reject via a TypeError path and break the toggle flow on unsupported runtimes. This is a direct platform-specific missing API regression risk.

  2. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (approx. lines 95-101, 131-145) — severity: warning
    frameReady is awaited even when video.play() fails. In failure/no-decode paths, the frame callback may never fire, so toggle() / end() can stall and leave UI state stuck. This is a timing/race contract issue: state transitions depend on an await that may never resolve.

Security Assessment

No security findings in this diff. Changes are isolated to onboarding special-page UI/CSS and do not modify message bridge, origin validation, captured globals, wrapper utilities, or injected feature shims.

Risk Level

High Risk — while scope is limited to onboarding, this introduces a new platform API dependency in the video transition path without capability checks, which can cause user-visible functional breakage on unsupported WebViews.

Recommendations

  1. Guard requestVideoFrameCallback with capability detection before use, and keep state transition logic functional when unavailable.
  2. Do not await frameReady after failed play(); ensure toggle/end always reach a deterministic state transition.
  3. Add integration coverage for: missing requestVideoFrameCallback and rejected video.play() to verify no stuck phase/reverse state.
Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for all 3 issues found in the latest run.

  • ✅ Fixed: Play failure causes indefinite hang via unresolved promise
    • Added playSucceeded flag to only await frameReady if video.play() succeeds, preventing indefinite hang when play fails.
  • ✅ Fixed: Stale closure in async toggle after await
    • Updated toggle function to use stateRef.current instead of closure state variable, matching the pattern used in end function.
  • ✅ Fixed: Auto-play timer mismatched with new fade-in delay
    • Updated timeout from 667ms to 767ms and comment to reflect the new 500ms fade-in delay (500ms + 267ms = 767ms).

Create PR

Or push these changes by commenting:

@cursor push 6134e28588
Preview (6134e28588)
diff --git a/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js b/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js
--- a/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js
+++ b/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js
@@ -93,12 +93,16 @@
         video.currentTime = 0;
         /** @type {Promise<void>} */
         const frameReady = new Promise((resolve) => video.requestVideoFrameCallback(() => resolve()));
+        let playSucceeded = false;
         try {
             await video.play();
+            playSucceeded = true;
         } catch (error) {
             console.error(error);
         }
-        await frameReady;
+        if (playSucceeded) {
+            await frameReady;
+        }
     };
 
     /** @param {HTMLVideoElement | null} video */
@@ -106,20 +110,20 @@
         if (video) video.currentTime = 0;
     };
 
-    // Auto-play after bubble entry animation (400ms delay + 267ms duration = 667ms)
+    // Auto-play after bubble entry animation (500ms delay + 267ms duration = 767ms)
     useEffect(() => {
         const id = setTimeout(
             () => {
                 play(videoFor('with'));
                 setState((prev) => ({ ...prev, phase: isReducedMotion ? 'settled' : 'playing' }));
             },
-            isReducedMotion ? 0 : 667,
+            isReducedMotion ? 0 : 767,
         );
         return () => clearTimeout(id);
     }, []); // exclude isReducedMotion from deps — must not re-fire if reduced motion changes after mount
 
     const toggle = async () => {
-        const { target, phase, reverse } = state;
+        const { target, phase, reverse } = stateRef.current;
         if (phase === 'initial') {
             // Queue or cancel a reverse so auto-play will switch to "without" once the "with" video ends
             setState({ target, phase, reverse: !reverse });

@noisysocks noisysocks marked this pull request as draft March 12, 2026 10:11
@noisysocks
Copy link
Contributor Author

@cursor Please make the 100ms delay between top bubble fade in and bottom bubble fade in configurable via url param. Similar to what we do for ?bubbleWidth.

@cursor
Copy link
Contributor

cursor bot commented Mar 12, 2026

Summary

Added a URL parameter bubbleFadeInDelay to configure the delay offset between the top and bottom bubble fade-in animations, following the same pattern as ?bubbleWidth.

Changes made:

  • Added bubbleFadeInDelay URL parameter parsing at the top of SingleStep.js
  • Updated the bottom bubble's fadeInDelay calculation to use: 400ms (default top delay) + configurable offset (default 100ms)

How it works:

  • Default behavior: Top bubble fades in at 400ms, bottom bubble at 500ms (100ms offset)
  • With ?bubbleFadeInDelay=200: Bottom bubble fades in at 600ms (200ms offset)
  • With ?bubbleFadeInDelay=0: Both bubbles fade in simultaneously at 400ms
  • Invalid values fall back to the default 100ms offset

The implementation matches the bubbleWidth override pattern and handles invalid input by falling back to the default.

Open in Web Open in Cursor 

Add ?bubbleFadeInDelay URL parameter to configure the delay offset between
top and bottom bubble fade-in animations. Default remains 100ms offset
(top: 400ms, bottom: 500ms). Follows the same pattern as ?bubbleWidth.

Co-authored-by: Robert Anderson <robert@noisysocks.com>
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Web Compatibility Assessment

  1. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (play() around requestVideoFrameCallback and await frameReady) — warning
    Unconditional video.requestVideoFrameCallback(...) use is a cross-platform compatibility risk (Missing APIs category). If this API is absent in any supported WebView/Safari baseline, it throws synchronously before the try/catch, causing toggle/end async handlers to reject.

  2. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (play() error path) — warning
    frameReady is awaited even when video.play() rejects. In rejection paths, no frame callback is guaranteed, so the promise can remain pending and stall state transitions (Timing and Race Conditions + return contract expectations for async handlers).

  3. special-pages/pages/onboarding/app/v4/components/Bubble.js + Bubble.module.css + SingleStep.js (fade delay plumbing) — info
    The staggered fade change is scoped to CSS custom-property timing and preserves component contracts. I do not see a site-compat regression pattern here.

  4. special-pages/pages/onboarding/app/v4/components/StepHeader.module.css (text alignment) — info
    Pure presentation change (text-align: left), no API/runtime compatibility concerns.

Security Assessment

No direct security findings in this diff.

The PR does not touch injected API shims, captured globals, messaging bridge, origin checks, or any network/data-exfiltration path. Changes are localized to onboarding special-page UI behavior.

Risk Level

Medium Risk — primarily due to the new first-frame synchronization path introducing potential runtime hangs/handler rejection on unsupported API or failed playback paths.

Recommendations

  1. Gate requestVideoFrameCallback with feature detection and provide a bounded fallback path (for example, resolve on loadeddata/canplay or immediate resolve after successful play()).
  2. Do not await frame readiness when play() fails; short-circuit to existing state transition logic so UI cannot deadlock.
  3. Add an automated test case that stubs missing requestVideoFrameCallback and a play() rejection to verify toggle/end still settle correctly.
Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

The "Hi there" and "Protections activated!" steps had centered
headings and body copy while the rest of the steps were left-aligned.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Web Compatibility Assessment

  1. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (lines 95-101) — error
    requestVideoFrameCallback is called unguarded (video.requestVideoFrameCallback(...)). On engines where this API is absent, this throws synchronously before video.play() runs, causing the toggle/end async handlers to reject and leaving the step stuck. This maps to the Missing APIs / platform-specific breakage risk.
  2. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (lines 95-101) — warning
    await frameReady runs even when video.play() rejects. If playback fails (decode/policy/media error), no frame callback is guaranteed, so the promise can remain pending and block state transitions. This creates a latent UI deadlock in the toggle flow.

Security Assessment

No security vulnerabilities found in this diff.
The changes are isolated to onboarding special-page UI/animation code and tests; no message bridge, transport/origin validation, captured-globals, network, or injected API-shimming paths were modified.

Risk Level

Medium Risk — scoped UI changes, but the new video-frame sync path introduces a real compatibility failure mode on unsupported or failed playback paths.

Recommendations

  1. Guard requestVideoFrameCallback with feature detection and fall back to immediate resolve or a loadeddata/canplay-based wait.
  2. Do not await frame readiness after failed play(); short-circuit to preserve state progress. A pragmatic alternative is Promise.race([frameReady, timeout]) with a short timeout.
  3. Add an integration/unit test path that stubs HTMLVideoElement.prototype.requestVideoFrameCallback as undefined and asserts toggle still progresses without hanging.
Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

noisysocks and others added 2 commits March 13, 2026 10:51
The comparison table and actions now fade in 100ms after the title,
configurable via the bubbleFadeInDelay URL param.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Web Compatibility Assessment

  1. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (95-101) — error
    video.requestVideoFrameCallback(...) is called unconditionally. On platforms/engines where this API is missing, this throws and breaks toggle/end transitions for the Duck Player step. This is a Missing APIs / platform-specific breakage risk.

  2. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (96-101, 133, 143) — error
    play() catches video.play() rejection but still awaits frameReady. If playback fails (or first frame is never produced), requestVideoFrameCallback may never resolve, causing state transitions to hang in async handlers (toggle/end). This is a timing/race/hang compatibility issue.

Security Assessment

No direct security vulnerabilities identified in this diff.

  • No changes to message bridge, origin validation, captured globals, wrapper utilities, or privileged transports.
  • New URL param usage (bubbleFadeInDelay) is parsed numerically before writing to CSS custom properties, so no direct script/CSS injection vector is introduced by these lines.

Risk Level

Medium Risk — UI-only scope, but the new video-frame synchronization path introduces cross-platform API availability and unresolved-promise failure modes that can break onboarding progression.

Recommendations

  1. Guard requestVideoFrameCallback with feature detection and provide a fallback (loadeddata/canplay/requestAnimationFrame) before visibility swap.
  2. Do not await frame readiness after video.play() rejection; fail fast to previous behavior (switch state without frame wait) or use a bounded timeout.
  3. Add tests for degraded environments: missing requestVideoFrameCallback and rejected video.play() to ensure toggle/end never deadlock.
Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

…mer mismatch

- Move `await frameReady` inside try block so it's skipped when play() fails
- Read from stateRef.current in toggle() to avoid stale closure after await
- Update auto-play timer from 667ms to 917ms to match new 650ms bubble fade delay

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@noisysocks noisysocks marked this pull request as ready for review March 13, 2026 04:05
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Web Compatibility Assessment

  1. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (~94-99) — warning
    requestVideoFrameCallback is invoked unguarded (video.requestVideoFrameCallback(...)) and the promise is created before the try/catch. On engines where this API is missing (or partially implemented), this throws/rejects before the guarded block, so play() can fail with no fallback frame-sync path. In this component, useEffect calls play(...) without awaiting/catching, so this can surface as unhandled rejection + broken onboarding video transition behavior on older WebView/Safari baselines.

  2. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.module.css + special-pages/pages/onboarding/integration-tests/onboarding.v4.spec.js (hidden styling and test assertion change) — info
    Switching from visibility: hidden to opacity: 0 + pointer-events: none is compatible with the new first-frame swap strategy and avoids the previous-frame flash. Test update to assert opacity is consistent with that contract.

Security Assessment

  1. No security vulnerabilities found in this diffinfo
    No changes touch injected-script wrapper utilities, captured globals, message bridge, transport origin checks, or config trust boundaries. No new exfiltration surfaces (postMessage('*'), eval/dynamic code, network calls) were introduced.

Risk Level

Medium Risk — UI-only scope, but the new hard dependency on requestVideoFrameCallback without capability fallback can regress behavior on unsupported or lagging embedded engines.

Recommendations

  1. Add a capability-gated fallback in play():
    Use if (typeof video.requestVideoFrameCallback === 'function') for the frame-sync path; otherwise fallback to requestAnimationFrame/loadeddata (or immediate state switch) so toggling still works.

  2. Move frame-ready setup inside guarded logic:
    Create frameReady inside try (or guard first) so unsupported API errors are captured and do not escape as unhandled async failures.

  3. Add a test for unsupported API behavior:
    Stub HTMLVideoElement.prototype.requestVideoFrameCallback = undefined in a focused test and verify toggle/settle still transitions correctly without flash/regression.

Alternative implementation:
Instead of waiting on requestVideoFrameCallback, switch after await video.play() plus one requestAnimationFrame tick on unsupported platforms; this keeps behavior deterministic while preserving compatibility.

Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Web Compatibility Assessment

  • special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (~96-98) — warning: video.requestVideoFrameCallback(...) is used without an existence check. On embedded engines where this API is missing/partial, frameReady rejects and falls into catch, which avoids a hard crash but still degrades behavior (reintroduces immediate visibility swap path) and can emit repeated console errors. This maps to the Missing APIs / platform quirks risk class.

Security Assessment

  • No direct security vulnerabilities found in this diff.
  • window.location.search overrides are parsed via Number.parseInt and only flow into CSS timing variables, so no obvious injection/exfiltration vector is introduced.

Risk Level

Low Risk — Changes are confined to onboarding special-page UI/timing/CSS and tests; no modifications to injected hostile-page wrappers, messaging bridge, captured globals, or security-critical initialization paths.

Recommendations

  1. Add a feature-detected fallback around requestVideoFrameCallback (e.g., resolve immediately or after a single requestAnimationFrame) to avoid platform-dependent rejections and noisy logs.
  2. Add/extend an integration test that runs the toggle flow with requestVideoFrameCallback unavailable (stubbed/undefined) and asserts no regression in state transitions or user-visible flashing behavior.
  3. Keep the new stagger-delay query-param override as test/debug-only and avoid relying on it for production timing logic.
Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web Compatibility Assessment

  1. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (lines 95-98) — warning
    requestVideoFrameCallback() is used unconditionally when building frameReady. This API is not guaranteed across all embedded WebView versions. On unsupported runtimes, the promise path throws/rejects and falls into the catch, which degrades back to immediate switching (reintroducing the flash risk this patch is trying to remove) and emits console errors during normal interaction.

  2. special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js (lines 121-145) — warning
    toggle() and end() are now async and both await play(...) before committing setState(...). That introduces an ordering window where multiple interactions/video-end events can resolve out of order and apply stale state. Rapid toggles can produce non-deterministic target/phase transitions (visual flicker or wrong active side).

Security Assessment

No direct security vulnerabilities found in this PR.

  • The diff does not modify injected API shims/wrappers, message bridge, captured globals, origin checks, or privileged transport paths.
  • New query-param usage for animation delay is parsed via Number.parseInt with NaN fallback and only feeds CSS timing values.

Risk Level

Medium Risk — UI-only scope, but the new video switching path depends on a non-universal media API and introduces async state-ordering complexity that can regress behavior on some platforms.

Recommendations

  1. Guard requestVideoFrameCallback with capability detection and use a deterministic fallback (loadeddata/seeked or one requestAnimationFrame after play()) so behavior is stable across all supported WebViews.
  2. Add a transition token/version check (or serialized queue) so stale async completions cannot overwrite newer user intent.
  3. Add integration coverage for:
  • rapid repeated toggle clicks while videos are playing
  • environments where HTMLVideoElement.prototype.requestVideoFrameCallback is unavailable
  • assertion that the hidden video is both visually hidden and non-interactive through transition boundaries.
Open in Web View Automation 

Sent by Cursor Automation: Web compat and sec

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Async toggle/end creates state machine race condition
    • Fixed by updating state immediately before async play() calls in both toggle() and end() functions, making state transitions atomic and preventing race conditions from concurrent events.
  • ✅ Fixed: Duplicated stagger delay calculation risks future divergence
    • Extracted the duplicated calculation into a shared utility function getBubbleFadeInStaggerDelay() in utils/bubbleDelay.js, ensuring both SingleStep.js and MakeDefaultContent.js use the same logic.

Create PR

Or push these changes by commenting:

@cursor push a233f9e309
Preview (a233f9e309)
diff --git a/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js b/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js
--- a/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js
+++ b/special-pages/pages/onboarding/app/v4/components/DuckPlayerContent.js
@@ -130,8 +130,9 @@
         } else {
             // Settled: play the other video, wait for its first frame, then switch visibility
             const next = flip(target);
+            // Update state immediately to make transition atomic and prevent race conditions
+            setState({ target: next, phase: isReducedMotion ? 'settled' : 'playing', reverse: false });
             await play(videoFor(next));
-            setState({ target: next, phase: isReducedMotion ? 'settled' : 'playing', reverse: false });
         }
     };
 
@@ -140,8 +141,9 @@
         if (reverse) {
             // A reverse was queued — play the other video, wait for its first frame, then switch
             const next = flip(target);
+            // Update state immediately to make transition atomic and prevent race conditions
+            setState({ target: next, phase: 'playing', reverse: false });
             await play(videoFor(next));
-            setState({ target: next, phase: 'playing', reverse: false });
         } else {
             // No reverse — just settle on the current video
             setState((prev) => ({ ...prev, phase: 'settled' }));

diff --git a/special-pages/pages/onboarding/app/v4/components/MakeDefaultContent.js b/special-pages/pages/onboarding/app/v4/components/MakeDefaultContent.js
--- a/special-pages/pages/onboarding/app/v4/components/MakeDefaultContent.js
+++ b/special-pages/pages/onboarding/app/v4/components/MakeDefaultContent.js
@@ -10,12 +10,10 @@
 import { useAnimate } from '../hooks/useAnimate';
 import { usePresence } from '../hooks/usePresence';
 import { useFlip } from '../hooks/useFlip';
+import { getBubbleFadeInStaggerDelay } from '../utils/bubbleDelay';
 import cn from 'classnames';
 import styles from './MakeDefaultContent.module.css';
 
-/** @type {string|null} */
-const bubbleFadeInDelayOverride = new URLSearchParams(window.location.search).get('bubbleFadeInDelay');
-
 /**
  * Top bubble content for the makeDefaultSingle step.
  * Shows title (changes after user makes default), comparison table, and Skip/Make Default buttons.
@@ -81,10 +79,7 @@
         })();
     };
 
-    const defaultBubbleDelay = 400;
-    const defaultOffset = 250;
-    const parsedOffset = bubbleFadeInDelayOverride ? Number.parseInt(bubbleFadeInDelayOverride, 10) : defaultOffset;
-    const staggerDelay = defaultBubbleDelay + (Number.isNaN(parsedOffset) ? defaultOffset : parsedOffset);
+    const staggerDelay = getBubbleFadeInStaggerDelay();
 
     return (
         <Container class={styles.root}>

diff --git a/special-pages/pages/onboarding/app/v4/components/SingleStep.js b/special-pages/pages/onboarding/app/v4/components/SingleStep.js
--- a/special-pages/pages/onboarding/app/v4/components/SingleStep.js
+++ b/special-pages/pages/onboarding/app/v4/components/SingleStep.js
@@ -3,14 +3,12 @@
 import cn from 'classnames';
 import { Bubble } from './Bubble';
 import { useStepConfig } from '../hooks/useStepConfig';
+import { getBubbleFadeInStaggerDelay } from '../utils/bubbleDelay';
 import styles from './SingleStep.module.css';
 
 /** @type {string|null} */
 const bubbleWidthOverride = new URLSearchParams(window.location.search).get('bubbleWidth');
 
-/** @type {string|null} */
-const bubbleFadeInDelayOverride = new URLSearchParams(window.location.search).get('bubbleFadeInDelay');
-
 /**
  * Main layout component for v4 steps.
  * Steps with bubbles use absolute positioning; the layout measures bubble heights.
@@ -70,16 +68,7 @@
                 exiting={globalState.exiting}
                 onExitComplete={topBubble ? undefined : advance}
                 progress={showProgress && !topBubble ? progress : undefined}
-                fadeInDelay={
-                    topBubble
-                        ? (() => {
-                              const defaultTopDelay = 400; // Default fade-in delay from CSS
-                              const defaultOffset = 250; // Default 250ms offset between top and bottom
-                              const offset = bubbleFadeInDelayOverride ? Number.parseInt(bubbleFadeInDelayOverride, 10) : defaultOffset;
-                              return defaultTopDelay + (Number.isNaN(offset) ? defaultOffset : offset);
-                          })()
-                        : undefined
-                }
+                fadeInDelay={topBubble ? getBubbleFadeInStaggerDelay() : undefined}
             >
                 {bottomBubble?.content}
             </Bubble>

diff --git a/special-pages/pages/onboarding/app/v4/utils/bubbleDelay.js b/special-pages/pages/onboarding/app/v4/utils/bubbleDelay.js
new file mode 100644
--- /dev/null
+++ b/special-pages/pages/onboarding/app/v4/utils/bubbleDelay.js
@@ -1,0 +1,13 @@
+/**
+ * Calculates the stagger delay for bottom bubble fade-in animation.
+ * The delay is computed from a base delay (400ms) plus an optional offset from URL params.
+ *
+ * @returns {number} The stagger delay in milliseconds
+ */
+export function getBubbleFadeInStaggerDelay() {
+    const defaultBubbleDelay = 400;
+    const defaultOffset = 250;
+    const bubbleFadeInDelayOverride = new URLSearchParams(window.location.search).get('bubbleFadeInDelay');
+    const parsedOffset = bubbleFadeInDelayOverride ? Number.parseInt(bubbleFadeInDelayOverride, 10) : defaultOffset;
+    return defaultBubbleDelay + (Number.isNaN(parsedOffset) ? defaultOffset : parsedOffset);
+}


const end = () => {
if (state.reverse) {
// A reverse was queued — play the other video now
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Async toggle/end creates state machine race condition

Medium Severity

Making toggle and end async introduces a gap between reading stateRef.current and calling setState, during which concurrent events can interleave and overwrite state. Previously these were synchronous — setState ran immediately after play (fire-and-forget), so the state machine stayed consistent. Now, in the settled branch of toggle, a rapid second click sees the same stale phase: 'settled' and re-enters the same branch, restarting the video via currentTime = 0. Worse, in end's reverse branch, if the user clicks toggle during await play(...), their intent to cancel the reverse gets silently overwritten when end's delayed setState runs with target: next.

Additional Locations (1)
Fix in Cursor Fix in Web

const defaultBubbleDelay = 400;
const defaultOffset = 250;
const parsedOffset = bubbleFadeInDelayOverride ? Number.parseInt(bubbleFadeInDelayOverride, 10) : defaultOffset;
const staggerDelay = defaultBubbleDelay + (Number.isNaN(parsedOffset) ? defaultOffset : parsedOffset);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated stagger delay calculation risks future divergence

Low Severity

The stagger delay calculation — parsing bubbleFadeInDelay URL param, applying a default offset of 250ms, and adding it to a base delay of 400ms — is duplicated identically in both SingleStep.js and MakeDefaultContent.js, including the same module-level URLSearchParams parse. Both also independently declare bubbleFadeInDelayOverride. If the default values or logic change in one file but not the other, they'll silently diverge.

Additional Locations (1)
Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-patch Bug fix / internal — no release needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants